test: cover informer retry after a CR deserialization problem - #3558
test: cover informer retry after a CR deserialization problem#3558csviri wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
csviri
changed the title
Adds an IT for the situation of ADR 002 where the resource that cannot be deserialized is already present when the informer lists on startup, the counterpart of MultiVersionCRDIT, which covers the watch case. With stopOnInformerErrorDuringStartup set to false the operator starts, but the informer is not retried: the exception handler installed by the informer pool declines a retry for deserialization errors and the Reflector of the fabric8 client then completes its stop future, so fixing the problem in the cluster while the operator runs does not bring the informer back. The test asserts that current behavior, so it fails once the informer does get retried; the log of the informer pool no longer promises a periodic retry in this case.
e211983 to
4e8a84c
Compare
csviri
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Pull request overview
This PR adds an integration test documenting current behavior when an informer fails to deserialize an already-present Custom Resource during the initial list-on-startup path (ADR 002 scenario), and adjusts runtime logging to avoid claiming periodic retries in a case where the informer won’t actually be retried.
Changes:
- Add
MultiVersionCRDDeserializationRetryITto cover deserialization failure during initial informer list and assert the informer does not recover after the offending CR is removed. - Update
AbstractInformerPool#startlogging to explicitly state that deserialization-related startup failures won’t be retried whenstopOnInformerErrorDuringStartup=false.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/multiversioncrd/MultiVersionCRDDeserializationRetryIT.java |
New IT documenting non-retry behavior for startup list deserialization failures and validating runtime health remains degraded. |
operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/pool/AbstractInformerPool.java |
Refines startup error logging to avoid promising retries for deserialization exceptions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * resources on startup. Since {@code stopOnInformerErrorDuringStartup} is {@code false} the | ||
| * operator starts anyway and logs that it will periodically retry the informer, so this test checks | ||
| * whether such a retry really happens, i.e. whether an operator picks it up when the problem is | ||
| * fixed in the cluster while it is running. |
| await() | ||
| .untilAsserted( | ||
| () -> | ||
| assertThat( | ||
| operator.get( | ||
| MultiVersionCRDTestCustomResource2.class, NOT_DESERIALIZABLE_CR_NAME)) | ||
| .isNull()); |
|
This correctly tests the current behavior, although, visiting the topic again, I'm not entirely convinced, if this is the right thing to do, so not to retry on deserialization error. But will stick with this for now, and will re-visit on demand. |
Adds an IT for the situation of ADR 002 where the resource that cannot be
deserialized is already present when the informer lists on startup, the
counterpart of MultiVersionCRDIT, which covers the watch case. With
stopOnInformerErrorDuringStartup set to false the operator starts, but the
informer is not retried: the exception handler installed by the informer pool
declines a retry for deserialization errors and the Reflector of the fabric8
client then completes its stop future, so fixing the problem in the cluster
while the operator runs does not bring the informer back.
The test asserts that current behavior, so it fails once the informer does get
retried; the log of the informer pool no longer promises a periodic retry in
this case.